Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add S2 keys support to zwave_js config #2157

Merged
merged 19 commits into from
Sep 29, 2021

Conversation

raman325
Copy link
Collaborator

@raman325 raman325 commented Aug 23, 2021

In order to support the new key structure for zwave-js, we need to add three new keys as inputs. We will also migrate keys out of network_key into s0_legacy_key to better align with this new structure.

This can't be merged until zwave-js/zwave-js-server#353 is released but opening it as a draft for input

fixes #2182

zwave_js/rootfs/etc/cont-init.d/config.sh Outdated Show resolved Hide resolved
zwave_js/rootfs/etc/cont-init.d/config.sh Outdated Show resolved Hide resolved
zwave_js/rootfs/etc/cont-init.d/config.sh Outdated Show resolved Hide resolved
@raman325 raman325 marked this pull request as ready for review August 25, 2021 06:25
zwave_js/DOCS.md Outdated Show resolved Hide resolved
@raman325
Copy link
Collaborator Author

one thought here - should we remove the autogenerate logic for the s2 keys since we are not ready to use them yet? we can keep most of the logic in place for when we are ready to generate them and it still allows users to switch between zwavejs2mqtt and zwave-js with S2 devices if they need to. It's also possible that users may not notice these new configuration options immediately and would want to generate keys on their own rather than trusting the addon (that's what I would want to do)

@MartinHjelmare
Copy link
Member

Are the S2 keys somehow stored in the controller device or other devices even if no devices are included with S2? Ie, does it matter if the user changes these keys later before starting to include devices using S2?

If the keys can be changed without consequences by the user before starting to include devices with S2, I don't think it matters if we auto-generate keys now.

@raman325
Copy link
Collaborator Author

Are the S2 keys somehow stored in the controller device or other devices even if no devices are included with S2? Ie, does it matter if the user changes these keys later before starting to include devices using S2?

If the keys can be changed without consequences by the user before starting to include devices with S2, I don't think it matters if we auto-generate keys now.

No and fair point

@raman325
Copy link
Collaborator Author

raman325 commented Sep 9, 2021

is there anything holding this PR back from being merged?

@MartinHjelmare
Copy link
Member

This looks ok, but I'd like someone that is familiar with bashio to approve before merge.

Copy link
Contributor

@kpine kpine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments are just about the docs. Docs are always opinionated, so leave, take or change my suggestions as you desire. 😃

zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/DOCS.md Outdated Show resolved Hide resolved
zwave_js/rootfs/etc/cont-init.d/config.sh Outdated Show resolved Hide resolved
Copy link
Member

@ludeeus ludeeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"How to use" and "Configuration" sections needs adjustments

@MartinHjelmare
Copy link
Member

We need to support the new keys in the integration add-on management too. I'll work on that tonight.

@MartinHjelmare
Copy link
Member

Here's the core PR to manage S2 keys also in the integration.
home-assistant/core#56783

It can be merged after we merge here and bump the client library used in the core (#2188).

@MartinHjelmare MartinHjelmare self-assigned this Sep 29, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs looks good. Someone with bashio skills should review that part.

@MartinHjelmare
Copy link
Member

Hmm, I'm getting this error when testing the add-on locally.

Error: Invalid networkKey defined

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Sep 29, 2021

I fixed the error. It was just a typo in the config template.

Tested and working! 🎉

@MartinHjelmare MartinHjelmare merged commit a414d16 into home-assistant:master Sep 29, 2021
@raman325 raman325 deleted the s2 branch November 16, 2021 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Z-Wave JS addon uses legacy configuration option "networkKey"
6 participants